Remove global structure in LCD HAL Layer #6597

abhinav-s235 · 2024-12-24T09:47:30Z

Replaced the global LCD HAL layer instance with a local declaration within the lcddev_register() function to reduce memory usage and passed this instance to LCD_FLUSH_THREAD by converting its address to a string using atoi() function.

jeongarmy · 2024-12-26T01:02:32Z

os/drivers/lcd/lcd_dev.c

-
-	int pid = kernel_thread("LCD Frame flusing", 204, 8192, lcd_flushing_thread, NULL);
+	itoa((int)lcd_info, parm_buf, 16);
+	parm[0] = parm_buf;


param[0] = itoa((int)lcd_info, parm_buf, 16);

And don't we need to memset parm_buf?

I don't know why we need to convert it to ascii and to reconvert to integer.
Is it possible to give the address itself without converting?

It is not possible to pass the address without converting it to string because kernel_thread() function accept the parameter of type FAR char *const argv[].
int kernel_thread(FAR const char *name, int priority, int stack_size, main_t entry, FAR char *const argv[])

param[0] = itoa((int)lcd_info, parm_buf, 16);

And don't we need to memset parm_buf?

I don't think it is needed
could you please explain why you think so?

sunghan-chang

Could you leave why you modify this in the COMMIT description? I can't get why you did.

sunghan-chang · 2024-12-26T02:41:56Z

os/drivers/lcd/lcd_dev.c

+	char *parm[2];
+	char parm_buf[9]; /* for storing 32 bit address */


The param and param_buf are difficult to distinguish and to know what they are. Could you rename them to know what they are in name?

The param and param_buf are difficult to distinguish and to know what they are. Could you rename them to know what they are in name?

fixed

sunghan-chang · 2024-12-26T02:44:55Z

os/drivers/lcd/lcd_dev.c

-
-	int pid = kernel_thread("LCD Frame flusing", 204, 8192, lcd_flushing_thread, NULL);
+	itoa((int)lcd_info, parm_buf, 16);
+	parm[0] = parm_buf;


I don't know why we need to convert it to ascii and to reconvert to integer.
Is it possible to give the address itself without converting?

sunghan-chang · 2024-12-26T06:21:08Z

Could you leave why you modify this in the COMMIT description? I can't get why you did.

@abhinav-s235 Could you check my comment above? I mean why you did below

Replaced the global LCD HAL layer instance with a local declaration within the lcddev_register function
and passed this instance to LCD_FLUSH_THREAD by converting its address to a string using atoi() function.

abhinav-s235 · 2024-12-26T06:26:48Z

Could you leave why you modify this in the COMMIT description? I can't get why you did.

Previously, while creating frame flushing thread we were not passing lcd_info pointer to function lcd_flushing_thread() that's why we declared it globally but now we are passing it as a parameter to the lcd_flushing_thread() function there is no need to globally declare it.

os/drivers/lcd/lcd_dev.c

sunghan-chang · 2024-12-26T06:59:48Z

os/drivers/lcd/lcd_dev.c

@@ -423,13 +424,15 @@ int lcddev_register(struct lcd_dev_s *dev)
 {
 	char devname[16] = { 0, };
 	int ret;
+	char *task_info[2];


How about flushing_thread_args?

@abhinav-s235 Could you check my comment above?

How about flushing_thread_args?

It has been changed

sunghan-chang · 2024-12-26T07:01:29Z

Could you leave why you modify this in the COMMIT description? I can't get why you did.

Previously, while creating frame flushing thread we were not passing lcd_info pointer to function lcd_flushing_thread() that's why we declared it globally but now we are passing it as a parameter to the lcd_flushing_thread() function there is no need to globally declare it.

The point is you want to remove the global variable. Right? Is that to reduce memory usages?
And Let's leave it at the COMMIT description.

abhinav-s235 · 2024-12-31T09:54:13Z

Could you leave why you modify this in the COMMIT description? I can't get why you did.

Previously, while creating frame flushing thread we were not passing lcd_info pointer to function lcd_flushing_thread() that's why we declared it globally but now we are passing it as a parameter to the lcd_flushing_thread() function there is no need to globally declare it.

The point is you want to remove the global variable. Right? Is that to reduce memory usages? And Let's leave it at the COMMIT description.

Commit description has been changed.

anjana348 · 2025-01-02T03:07:47Z

os/drivers/lcd/lcd_dev.c

 	}
+cleanup:
+lcddbg("ERROR: Failed to register driver %s\n", devname);


indentation should be tab

sunghan-chang · 2025-01-02T09:25:26Z

@abhinav-s235 Is this verified with real device?

abhinav-s235 · 2025-01-02T09:39:07Z

@abhinav-s235 Is this verified with real device?

Yes

ewoodev · 2025-01-02T05:51:34Z

os/drivers/lcd/lcd_dev.c


 	if (!dev) {
 		return -EINVAL;
 	}

 	/* Allocate a new lcd_dev driver instance */
-	lcd_info = (struct lcd_s *)kmm_zalloc(sizeof(struct lcd_s));
+	struct lcd_s *lcd_info = (struct lcd_s *)kmm_zalloc(sizeof(struct lcd_s));


Let's move Variable declaration on top

ewoodev · 2025-01-02T09:41:35Z

os/drivers/lcd/lcd_dev.c

+	itoa((int)lcd_info, lcd_info_addr, 16);
+	flushing_thread_args[0] = lcd_info_addr;
+	flushing_thread_args[1] = NULL;
+	int pid = kernel_thread("LCD Frame flusing", 204, 8192, lcd_flushing_thread, (FAR char *const *)flushing_thread_args);


It also, Let's move variable declaration on top

sunghan-chang · 2025-01-03T03:04:19Z

os/drivers/lcd/lcd_dev.c

+#endif
+	struct lcd_s *lcd_info;
+	char *flushing_thread_args[2];
+	char lcd_info_addr[9]; /* for storing 32 bit address */


This should be in the CONFIG_LCD_FLUSH_THREAD conditional.
If not, when we disable the config, it will be unused variable warning.

sunghan-chang · 2025-01-03T03:06:36Z

os/drivers/lcd/lcd_dev.c

@@ -441,12 +448,19 @@ int lcddev_register(struct lcd_dev_s *dev)
 	lcd_info->empty = true;
 	lcd_info->lcd_kbuffer = (uint8_t *)kmm_malloc(CONFIG_LCD_XRES * CONFIG_LCD_YRES * sizeof(uint16_t));


The members - do_wait, empty and lcd_kbuffer are valid with the config - CONFIG_LCD_FLUSH_THREAD only?
When the config is disable, they are not used?

Yes, these members are only valid when config CONFIG_LCD_FLUSH_THREAD is enabled
struct lcd_s

…de lcddev_register function Replaced the global LCD HAL layer instance with a local declaration within the lcddev_register function to reduce memory usage and passed this instance to LCD_FLUSH_THREAD by converting its address to a string using atoi() function.

abhinav-s235 changed the title ~~os/drivers/lcd/lcd_dev.c: Declare LCD HAL layer instance locally inside lcddev_register() function~~ Remove global structure in LCD HAL Layer Dec 24, 2024

jeongarmy reviewed Dec 26, 2024

View reviewed changes

sunghan-chang reviewed Dec 26, 2024

View reviewed changes

abhinav-s235 force-pushed the lcd_flushing_thread branch from 3d5064e to 0229ed0 Compare December 26, 2024 06:07

sunghan-chang reviewed Dec 26, 2024

View reviewed changes

abhinav-s235 force-pushed the lcd_flushing_thread branch from 0229ed0 to 3668df6 Compare December 31, 2024 09:53

anjana348 reviewed Jan 2, 2025

View reviewed changes

abhinav-s235 force-pushed the lcd_flushing_thread branch from 3668df6 to ba637df Compare January 2, 2025 06:32

sunghan-chang previously approved these changes Jan 2, 2025

View reviewed changes

ewoodev reviewed Jan 2, 2025

View reviewed changes

abhinav-s235 dismissed sunghan-chang’s stale review via f74e644 January 2, 2025 10:32

abhinav-s235 force-pushed the lcd_flushing_thread branch from ba637df to f74e644 Compare January 2, 2025 10:32

sunghan-chang reviewed Jan 3, 2025

View reviewed changes

abhinav-s235 force-pushed the lcd_flushing_thread branch from f74e644 to c0638bb Compare January 3, 2025 04:24

sunghan-chang approved these changes Jan 3, 2025

View reviewed changes

sunghan-chang merged commit f36c01e into Samsung:master Jan 3, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove global structure in LCD HAL Layer #6597

Remove global structure in LCD HAL Layer #6597

abhinav-s235 commented Dec 24, 2024 •

edited

Loading

jeongarmy Dec 26, 2024

sunghan-chang Dec 26, 2024

abhinav-s235 Dec 26, 2024 •

edited

Loading

abhinav-s235 Dec 26, 2024

sunghan-chang left a comment

sunghan-chang Dec 26, 2024

abhinav-s235 Dec 26, 2024

sunghan-chang Dec 26, 2024

sunghan-chang commented Dec 26, 2024

abhinav-s235 commented Dec 26, 2024

sunghan-chang Dec 26, 2024

sunghan-chang Dec 31, 2024

abhinav-s235 Dec 31, 2024

sunghan-chang commented Dec 26, 2024

abhinav-s235 commented Dec 31, 2024

anjana348 Jan 2, 2025

abhinav-s235 Jan 2, 2025

sunghan-chang commented Jan 2, 2025

abhinav-s235 commented Jan 2, 2025

ewoodev Jan 2, 2025

abhinav-s235 Jan 2, 2025

ewoodev Jan 2, 2025

abhinav-s235 Jan 2, 2025

sunghan-chang Jan 3, 2025

abhinav-s235 Jan 3, 2025

sunghan-chang Jan 3, 2025

abhinav-s235 Jan 3, 2025

		char *parm[2];
		char parm_buf[9]; /* for storing 32 bit address */

		@@ -441,12 +448,19 @@ int lcddev_register(struct lcd_dev_s *dev)
		lcd_info->empty = true;
		lcd_info->lcd_kbuffer = (uint8_t )kmm_malloc(CONFIG_LCD_XRES CONFIG_LCD_YRES * sizeof(uint16_t));

Remove global structure in LCD HAL Layer #6597

Remove global structure in LCD HAL Layer #6597

Conversation

abhinav-s235 commented Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav-s235 Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang commented Dec 26, 2024

abhinav-s235 commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang commented Dec 26, 2024

abhinav-s235 commented Dec 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang commented Jan 2, 2025

abhinav-s235 commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav-s235 commented Dec 24, 2024 •

edited

Loading

abhinav-s235 Dec 26, 2024 •

edited

Loading